Skip to content

refactor: modernize build & test system - #75

Merged
sbillinge merged 4 commits into
diffpy:mainfrom
SYMSCAE:cmake-build
Nov 6, 2026
Merged

sbillinge merged 4 commits into
diffpy:mainfrom
SYMSCAE:cmake-build

Conversation

@SYMSCAE

@SYMSCAE SYMSCAE commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

This PR aims to modernize build system and tests by switching build system from scons to Cmake + Ninja and test system from cxxtests to Catch2.

linux-x86_64 release build benchmarks:

  • one-file change incremental rebuild: 1.44s -> 1.13s
  • rebuild after cache: 411ms -> 23ms

@SYMSCAE
SYMSCAE marked this pull request as ready for review November 5, 2026 21:31
@SYMSCAE

SYMSCAE commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

@sbillinge PR ready for review. Please see if we want this, this will improve quite a lot for incremental builds.

@sbillinge

Copy link
Copy Markdown
Contributor

@SYMSCAE thanks for this. I asked Claude Fable for help and this is what I got. Looks good but one thing to fix and some suggestions.

● Thanks, this is a careful port and I would like to merge it. The CMake and Catch2 work is of high quality, the version
logic is a faithful port of libdiffpybuildutils.py (git describe on this branch gives 1.5.0-3-g24788bc, which
resolves to 1.5.0.post3 as before), and a suite-by-suite audit of the test conversion found every test name, assertion
and tolerance preserved 1:1 across all 26 suites. tests-on-pr with coverage passes.

A few things need to happen before merging, then some suggestions, mostly about where this is heading now that
diffpy.srreal vendors libdiffpy.

Before merging

  1. This breaks diffpy.srreal's build. srreal's CMakeLists.txt reads the base version from the submodule file
    site_scons/fallback_version.py (file(STRINGS ... REGEX "^FALLBACK_VERSION =")), and this PR deletes that file. The
    next submodule bump will fail srreal's configure in any git checkout. Please open a companion srreal PR that reads
    cmake/FallbackVersion.cmake instead (or, better, see the add_subdirectory suggestion below), and we merge that side
    first.
  2. Windows and macOS have not been exercised. Only tests-on-pr ran. The Windows path (Library/ prefix, DLL in
    bin/, PATH injection for installed tests, MSVC through Ninja) is the riskiest new code. I will dispatch the matrix
    workflow on this branch; please hold until it is green.
  3. Please link an issue. Open one describing the SCons to CMake move and add closes #N to the description.

Making it vendor-friendly

srreal currently duplicates the source glob, the C++ standard, the ObjCryst exclusion, the runtime-data install and the
full version resolution. The end state we want is add_subdirectory(extern/libdiffpy) in srreal so all of that lives
here. This CMakeLists is close, but a few things would need to change, and it is cheaper to do them now:

  • Guard the global settings with if(PROJECT_IS_TOP_LEVEL): the forced CMAKE_BUILD_TYPE, the CMAKE_INSTALL_PREFIX
    override from CONDA_PREFIX, the CMAKE_*_OUTPUT_DIRECTORY overrides and the global add_compile_options(-Wall -ffast-math). As written they leak into a parent project.
  • Add a LIBDIFFPY_INSTALL option so a vendoring parent can skip the install rules and the exported package.
  • Allow a static or OBJECT library (an option, or honour BUILD_SHARED_LIBS). srreal wheels link Boost and GSL
    statically.
  • Make LIBDIFFPY_RUNTIME_RELPATH a cache variable. srreal installs the data as diffpy/srreal/data and compiles with
    DIFFPYRUNTIMERELPATH=data.

Two related notes. Boost.Serialization stays a required public dependency of the C++ library, so the nanobind move
removes Boost.Python but not Boost. And libobjcryst still has no CMake build, which is why this PR needs a hand-written
cmake/FindObjCryst.cmake. Giving libobjcryst the same treatment would let that module go away and let pyobjcryst
find_package it.

Smaller comments

  • src/tests/CMakeLists.txt: the last commit (24788bc) replaced catch_discover_tests with one add_test(NAME alltests ...), so CTest now reports a single test and ctest -R Lattice no longer selects anything. The commit message does not
    say why. If discovery was too slow, one add_test per suite that runs alltests "[Tag]" keeps per-suite reporting and
    filtering at no discovery cost.
  • src/tests/TestOverlapCalculator.cpp: the old setUp called setAbortTestOnFail(true), so a failed size check
    stopped the test. With CHECK the following indexing can now run out of range. Please make the guard assertions
    REQUIRE.
  • src/tests/TestPQEvaluator.cpp: the enum value CHECK sits next to Catch2's CHECK macro (CHECK(CHECK == ...)). It
    compiles because the macro is function-like, but it is fragile. Qualify or alias the enum.
  • .github/workflows/tests-on-pr.yml: -DCMAKE_INSTALL_LIBDIR=lib/x86_64-linux-gnu is unexplained and
    -DCMAKE_BUILD_TYPE=Debug is redundant with the coverage preset. Add a comment or drop them.
  • news/cmake-build.rst: keep all the headings from news/TEMPLATE.rst with their placeholder text; only fill the ones
    that apply.
  • The shared cachedTestCrystal is fine: every consumer takes a const Crystal& and createStructureAdapter only calls
    const methods.
  • src/tests/objcryst_helpers.cpp: throwing on a missing or empty CIF and the RAII QuietObjCryst guard are nice
    improvements.

Build instructions, release checklist, docs and .readthedocs.yaml all look updated consistently. Nice work.

@SYMSCAE

SYMSCAE commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@sbillinge Please see the updated commit. mac & windows setup have been tested on matrix https://github.com/SYMSCAE/libdiffpy/actions/runs/42257163404/job/128027306216.

@sbillinge
sbillinge merged commit e7b105e into diffpy:main Nov 6, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants